[tarball-helper] Add tarball/manual build helper CLI and tests (issue #452) #641
Conversation
aybanda
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded a new top-level CLI command Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant TarballHelper
participant FS as File_System
participant Apt as apt-get
participant Tracker as JSON_Tracker
User->>CLI: tarball-helper <action> <path>
alt analyze
CLI->>TarballHelper: analyze(path)
TarballHelper->>FS: read build files
FS-->>TarballHelper: file contents
TarballHelper->>TarballHelper: parse deps & suggest apt packages
TarballHelper-->>CLI: deps + suggestions
CLI-->>User: display table
else install-deps
CLI->>TarballHelper: install_deps(path)
TarballHelper->>TarballHelper: analyze(path)
TarballHelper->>Apt: apt-get install pkg...
Apt-->>TarballHelper: install results
TarballHelper->>Tracker: track successful packages
Tracker->>FS: persist manual_builds.json
TarballHelper-->>CLI: install summary
CLI-->>User: display progress
else cleanup
CLI->>TarballHelper: cleanup()
TarballHelper->>Tracker: load tracked packages
Tracker->>FS: read manual_builds.json
TarballHelper->>Apt: apt-get remove pkg... (loop)
Apt-->>TarballHelper: removal results
TarballHelper->>Tracker: save updated list
Tracker->>FS: update manual_builds.json
TarballHelper-->>CLI: cleanup complete
CLI-->>User: confirmation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @aybanda, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Cortex CLI by integrating a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new tarball-helper CLI command for managing dependencies in manual builds, along with corresponding tests. The implementation is a good start, but there are some critical issues in the CLI integration and robustness concerns in the dependency parsing logic that need to be addressed. Specifically, there's duplicated code in cortex/cli.py that will cause the application to crash, and the dependency parsing in cortex/tarball_helper.py is fragile. I've provided detailed comments and suggestions to resolve these issues.
cortex/cli.py
Outdated
| # Register tarball-helper as a subparser command | ||
| tarball_parser = subparsers.add_parser( | ||
| "tarball-helper", help="Tarball/manual build helper (analyze, install-deps, cleanup)" | ||
| ) | ||
| tarball_parser.add_argument( | ||
| "action", choices=["analyze", "install-deps", "cleanup"], help="Action to perform" | ||
| ) | ||
| tarball_parser.add_argument( | ||
| "path", nargs="?", help="Path to source directory (for analyze/install-deps)" | ||
| ) | ||
|
|
||
| # Handle tarball-helper command | ||
| if args.command == "tarball-helper": | ||
| from rich.console import Console | ||
| from rich.table import Table | ||
|
|
||
| from cortex.tarball_helper import TarballHelper | ||
|
|
||
| helper = TarballHelper() | ||
| if args.action == "analyze": | ||
| deps = helper.analyze(args.path or ".") | ||
| mapping = helper.suggest_apt_packages(deps) | ||
| table = Table(title="Suggested apt packages") | ||
| table.add_column("Dependency") | ||
| table.add_column("Apt Package") | ||
| for dep, pkg in mapping.items(): | ||
| table.add_row(dep, pkg) | ||
| Console().print(table) | ||
| elif args.action == "install-deps": | ||
| deps = helper.analyze(args.path or ".") | ||
| mapping = helper.suggest_apt_packages(deps) | ||
| helper.install_deps(list(mapping.values())) | ||
| elif args.action == "cleanup": | ||
| helper.cleanup() | ||
| return 0 |
There was a problem hiding this comment.
This block of code has two major issues:
- Duplicated Parser Definition: It re-defines the
tarball-helpersubparser, which is already correctly defined at lines 3268-3277. Adding a parser afterparser.parse_args()(line 3912) will cause the application to crash with anargparse.ArgumentError. This entire definition block should be removed. - Misplaced Handler Logic: The handler logic for the
tarball-helpercommand is placed outside the main command routingtry...exceptblock. It should be moved inside that block (which starts around line 3970) to be consistent with other command handlers.
| patterns = { | ||
| "CMakeLists.txt": r"find_package\((\w+)", | ||
| "meson.build": r"dependency\(['\"](\w+)", | ||
| "configure.ac": r"AC_CHECK_LIB\(\[?(\w+)", | ||
| "Makefile": r"-l(\w+)", | ||
| "setup.py": r"install_requires=\[(.*?)\]", | ||
| } |
There was a problem hiding this comment.
The regex-based parsing for dependencies has a couple of issues:
- Fragile
setup.pyparsing: Using regex forsetup.py'sinstall_requiresis brittle and can fail with different code formatting (e.g., multi-line lists, comments). A more robust approach is to use Python'sastmodule to safely parse the file and extract the dependency list. - Restrictive patterns: The
\w+in the regex forCMakeLists.txt,meson.build, andconfigure.acdoesn't allow for hyphens in package names, which is common. This should be changed to[\w-]+to correctly capture names likelibcurl-dev.
| patterns = { | |
| "CMakeLists.txt": r"find_package\((\w+)", | |
| "meson.build": r"dependency\(['\"](\w+)", | |
| "configure.ac": r"AC_CHECK_LIB\(\[?(\w+)", | |
| "Makefile": r"-l(\w+)", | |
| "setup.py": r"install_requires=\[(.*?)\]", | |
| } | |
| patterns = { | |
| "CMakeLists.txt": r"find_package\(([-\w]+)", | |
| "meson.build": r"dependency\(['\"]([-\w]+)", | |
| "configure.ac": r"AC_CHECK_LIB\(\[?([-\w]+)", | |
| "Makefile": r"-l([-\w]+)", | |
| "setup.py": r"install_requires=\[(.*?)\]", | |
| } |
| """ | ||
| Unit tests for tarball_helper.py | ||
| """ | ||
|
|
||
| import json | ||
| import os | ||
| import shutil | ||
| import tempfile | ||
|
|
||
| import pytest | ||
|
|
||
| from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper | ||
|
|
||
|
|
||
| def test_analyze_cmake(tmp_path): | ||
| cmake = tmp_path / "CMakeLists.txt" | ||
| cmake.write_text(""" | ||
| find_package(OpenSSL) | ||
| find_package(ZLIB) | ||
| """) | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"OpenSSL", "ZLIB"} | ||
|
|
||
|
|
||
| def test_analyze_meson(tmp_path): | ||
| meson = tmp_path / "meson.build" | ||
| meson.write_text("dependency('libcurl')\ndependency('zlib')") | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"libcurl", "zlib"} | ||
|
|
||
|
|
||
| def test_suggest_apt_packages(): | ||
| helper = TarballHelper() | ||
| mapping = helper.suggest_apt_packages(["OpenSSL", "zlib"]) | ||
| assert mapping["OpenSSL"] == "libopenssl-dev" | ||
| assert mapping["zlib"] == "libzlib-dev" | ||
|
|
||
|
|
||
| def test_track_and_cleanup(tmp_path, monkeypatch): | ||
| # Patch MANUAL_TRACK_FILE to a temp location | ||
| test_file = tmp_path / "manual_builds.json" | ||
| monkeypatch.setattr("cortex.tarball_helper.MANUAL_TRACK_FILE", test_file) | ||
| helper = TarballHelper() | ||
| helper.track("libfoo-dev") | ||
| assert "libfoo-dev" in helper.tracked_packages | ||
| # Simulate cleanup (mock subprocess) | ||
| monkeypatch.setattr("subprocess.run", lambda *a, **k: None) | ||
| helper.cleanup() | ||
| assert helper.tracked_packages == [] | ||
| with open(test_file) as f: | ||
| data = json.load(f) | ||
| assert data["packages"] == [] |
There was a problem hiding this comment.
The added tests are a good start for the tarball-helper functionality. However, they don't cover some of the more complex and fragile parts of the implementation, such as parsing setup.py files. Given that the current regex-based approach for setup.py is brittle, adding tests for various install_requires formats (e.g., with comments, multiline, different quoting) would be highly beneficial to prevent future regressions and ensure correctness.
There was a problem hiding this comment.
Pull request overview
This PR adds a tarball/manual build helper CLI tool to assist users in analyzing build files, installing dependencies, and managing cleanup of manually installed packages. The PR also includes code formatting improvements across multiple files to standardize multi-line string declarations.
Changes:
- New
TarballHelperclass with dependency analysis, package suggestion, installation, and cleanup features - CLI integration for
tarball-helpercommand with analyze, install-deps, and cleanup actions - Unit tests covering basic functionality
- Code formatting improvements (multi-line string consolidation) across 11 existing files
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/tarball_helper.py | New tarball helper module with build file analysis and package management |
| tests/test_tarball_helper.py | Unit tests for tarball helper functionality |
| cortex/cli.py | Integration of tarball-helper command into main CLI |
| tests/test_shell_env_analyzer.py | Code formatting: consolidated multi-line strings |
| tests/test_role_management.py | Code formatting: consolidated multi-line strings |
| tests/test_context_memory.py | Code formatting: consolidated multi-line strings |
| cortex/uninstall_impact.py | Code formatting: split long string literal |
| cortex/transaction_history.py | Code formatting: consolidated multi-line strings |
| cortex/semantic_cache.py | Code formatting: consolidated multi-line strings |
| cortex/role_manager.py | Code formatting: consolidated multi-line strings |
| cortex/licensing.py | Code formatting: consolidated multi-line strings |
| cortex/kernel_features/model_lifecycle.py | Code formatting: consolidated multi-line strings |
| cortex/kernel_features/kv_cache_manager.py | Code formatting: consolidated multi-line strings |
| cortex/installation_history.py | Code formatting: consolidated multi-line strings |
| cortex/graceful_degradation.py | Code formatting: consolidated multi-line strings |
| cortex/first_run_wizard.py | Code formatting: consolidated multi-line strings |
| cortex/context_memory.py | Code formatting: consolidated multi-line strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cortex/tarball_helper.py
Outdated
| import os | ||
| import re | ||
| from pathlib import Path | ||
| from typing import Optional |
There was a problem hiding this comment.
The Optional type is imported from typing but is never used in the code. This unused import should be removed to keep the code clean.
| from typing import Optional |
| """ | ||
| Unit tests for tarball_helper.py | ||
| """ | ||
|
|
||
| import json | ||
| import os | ||
| import shutil | ||
| import tempfile | ||
|
|
||
| import pytest | ||
|
|
||
| from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper | ||
|
|
||
|
|
||
| def test_analyze_cmake(tmp_path): | ||
| cmake = tmp_path / "CMakeLists.txt" | ||
| cmake.write_text(""" | ||
| find_package(OpenSSL) | ||
| find_package(ZLIB) | ||
| """) | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"OpenSSL", "ZLIB"} | ||
|
|
||
|
|
||
| def test_analyze_meson(tmp_path): | ||
| meson = tmp_path / "meson.build" | ||
| meson.write_text("dependency('libcurl')\ndependency('zlib')") | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"libcurl", "zlib"} | ||
|
|
||
|
|
||
| def test_suggest_apt_packages(): | ||
| helper = TarballHelper() | ||
| mapping = helper.suggest_apt_packages(["OpenSSL", "zlib"]) | ||
| assert mapping["OpenSSL"] == "libopenssl-dev" | ||
| assert mapping["zlib"] == "libzlib-dev" | ||
|
|
||
|
|
||
| def test_track_and_cleanup(tmp_path, monkeypatch): | ||
| # Patch MANUAL_TRACK_FILE to a temp location | ||
| test_file = tmp_path / "manual_builds.json" | ||
| monkeypatch.setattr("cortex.tarball_helper.MANUAL_TRACK_FILE", test_file) | ||
| helper = TarballHelper() | ||
| helper.track("libfoo-dev") | ||
| assert "libfoo-dev" in helper.tracked_packages | ||
| # Simulate cleanup (mock subprocess) | ||
| monkeypatch.setattr("subprocess.run", lambda *a, **k: None) | ||
| helper.cleanup() | ||
| assert helper.tracked_packages == [] | ||
| with open(test_file) as f: | ||
| data = json.load(f) | ||
| assert data["packages"] == [] |
There was a problem hiding this comment.
There are no tests for the install_deps() method, which is a critical part of the functionality. Tests should verify: 1) that apt-get install is called with the correct arguments, 2) that packages are tracked after installation, 3) behavior when installations fail (given check=False), and 4) that output messages are displayed correctly.
| """ | ||
| Unit tests for tarball_helper.py | ||
| """ | ||
|
|
||
| import json | ||
| import os | ||
| import shutil | ||
| import tempfile | ||
|
|
||
| import pytest | ||
|
|
||
| from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper | ||
|
|
||
|
|
||
| def test_analyze_cmake(tmp_path): | ||
| cmake = tmp_path / "CMakeLists.txt" | ||
| cmake.write_text(""" | ||
| find_package(OpenSSL) | ||
| find_package(ZLIB) | ||
| """) | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"OpenSSL", "ZLIB"} | ||
|
|
||
|
|
||
| def test_analyze_meson(tmp_path): | ||
| meson = tmp_path / "meson.build" | ||
| meson.write_text("dependency('libcurl')\ndependency('zlib')") | ||
| helper = TarballHelper() | ||
| deps = helper.analyze(str(tmp_path)) | ||
| assert set(deps) == {"libcurl", "zlib"} | ||
|
|
||
|
|
||
| def test_suggest_apt_packages(): | ||
| helper = TarballHelper() | ||
| mapping = helper.suggest_apt_packages(["OpenSSL", "zlib"]) | ||
| assert mapping["OpenSSL"] == "libopenssl-dev" | ||
| assert mapping["zlib"] == "libzlib-dev" | ||
|
|
||
|
|
||
| def test_track_and_cleanup(tmp_path, monkeypatch): | ||
| # Patch MANUAL_TRACK_FILE to a temp location | ||
| test_file = tmp_path / "manual_builds.json" | ||
| monkeypatch.setattr("cortex.tarball_helper.MANUAL_TRACK_FILE", test_file) | ||
| helper = TarballHelper() | ||
| helper.track("libfoo-dev") | ||
| assert "libfoo-dev" in helper.tracked_packages | ||
| # Simulate cleanup (mock subprocess) | ||
| monkeypatch.setattr("subprocess.run", lambda *a, **k: None) | ||
| helper.cleanup() | ||
| assert helper.tracked_packages == [] | ||
| with open(test_file) as f: | ||
| data = json.load(f) | ||
| assert data["packages"] == [] |
There was a problem hiding this comment.
There is no test coverage for error conditions such as: 1) corrupted JSON in manual_builds.json, 2) missing or invalid path arguments to analyze(), 3) file read errors in analyze(), and 4) permission errors when saving tracked packages. These are important edge cases that should be tested.
| Usage: | ||
| cortex tarball-helper analyze <path> | ||
| cortex tarball-helper install-deps <path> | ||
| cortex tarball-helper track <package> | ||
| cortex tarball-helper cleanup |
There was a problem hiding this comment.
The usage docstring at the top mentions "cortex tarball-helper track " but there is no "track" action defined in the CLI argument parser (only analyze, install-deps, and cleanup are valid choices). Either the docstring should be updated to remove this, or the track action should be implemented.
cortex/tarball_helper.py
Outdated
| def cleanup(self): | ||
| import subprocess | ||
|
|
||
| for pkg in self.tracked_packages: | ||
| console.print(f"[yellow]Removing:[/yellow] {pkg}") | ||
| subprocess.run(["sudo", "apt-get", "remove", "-y", pkg], check=False) | ||
| self.tracked_packages = [] | ||
| self._save_tracked_packages() | ||
| console.print("[green]Cleanup complete.[/green]") |
There was a problem hiding this comment.
The cleanup() method doesn't prompt the user for confirmation before removing packages. This could lead to accidental removal of packages. Consider adding a confirmation prompt, especially since this command uses sudo and could affect system packages.
cortex/tarball_helper.py
Outdated
| subprocess.run(["sudo", "apt-get", "install", "-y", pkg], check=False) | ||
| self.track(pkg) |
There was a problem hiding this comment.
The install_deps() method calls subprocess.run with check=False, meaning installation failures are silently ignored. Failed installations are still tracked for later cleanup, which can cause issues. Consider either: 1) checking the return code and only tracking successful installations, or 2) logging/warning the user when an installation fails.
| subprocess.run(["sudo", "apt-get", "install", "-y", pkg], check=False) | |
| self.track(pkg) | |
| result = subprocess.run(["sudo", "apt-get", "install", "-y", pkg], check=False) | |
| if result.returncode == 0: | |
| self.track(pkg) | |
| else: | |
| console.print( | |
| f"[red]Failed to install:[/red] {pkg} (exit code {result.returncode}). " | |
| "Package will not be tracked for cleanup." | |
| ) |
cortex/tarball_helper.py
Outdated
| with open(MANUAL_TRACK_FILE) as f: | ||
| return json.load(f).get("packages", []) |
There was a problem hiding this comment.
The _load_tracked_packages() method does not handle JSON decode errors. If the manual_builds.json file is corrupted or contains invalid JSON, the function will raise an unhandled JSONDecodeError, causing the entire TarballHelper initialization to fail.
| with open(MANUAL_TRACK_FILE) as f: | |
| return json.load(f).get("packages", []) | |
| try: | |
| with open(MANUAL_TRACK_FILE) as f: | |
| data = json.load(f) | |
| except json.JSONDecodeError: | |
| console.print( | |
| f"[yellow]Warning:[/yellow] Failed to parse {MANUAL_TRACK_FILE}. " | |
| "Ignoring corrupt tracking data." | |
| ) | |
| return [] | |
| packages = data.get("packages", []) | |
| if not isinstance(packages, list): | |
| return [] | |
| return packages |
cortex/tarball_helper.py
Outdated
|
|
||
| for pkg in self.tracked_packages: | ||
| console.print(f"[yellow]Removing:[/yellow] {pkg}") | ||
| subprocess.run(["sudo", "apt-get", "remove", "-y", pkg], check=False) |
There was a problem hiding this comment.
The cleanup() method uses 'apt-get remove' which leaves configuration files behind. For a complete cleanup, consider using 'apt-get purge' instead, or at least document this behavior so users understand that configuration files may remain after cleanup.
| subprocess.run(["sudo", "apt-get", "remove", "-y", pkg], check=False) | |
| subprocess.run(["sudo", "apt-get", "purge", "-y", pkg], check=False) |
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
tests/test_tarball_helper.py
Outdated
|
|
||
| import pytest | ||
|
|
||
| from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper |
There was a problem hiding this comment.
Import of 'MANUAL_TRACK_FILE' is not used.
| from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper | |
| from cortex.tarball_helper import TarballHelper |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/installation_history.py (1)
315-364: Critical bug: Database operations performed outside connection context manager.The
update_installationmethod has a scoping issue. Thewith self._pool.get_connection() as conn:block ends at line 333 (after fetching the result), but subsequent operations usingcursor(lines 341-357) andconn.commit()(line 359) occur outside the context manager. This will fail because the connection has already been released back to the pool.🐛 Proposed fix
def update_installation( self, install_id: str, status: InstallationStatus, error_message: str | None = None ): """Update installation record after completion""" try: with self._pool.get_connection() as conn: cursor = conn.cursor() # Get packages from record cursor.execute( "SELECT packages, timestamp FROM installations WHERE id = ?", (install_id,) ) result = cursor.fetchone() if not result: logger.error(f"Installation {install_id} not found") return packages = json.loads(result[0]) - start_time = datetime.datetime.fromisoformat(result[1]) - duration = (datetime.datetime.now() - start_time).total_seconds() - - # Create after snapshot - after_snapshot = self._create_snapshot(packages) - - # Update record - cursor.execute( - """ - UPDATE installations - SET status = ?, - after_snapshot = ?, - error_message = ?, - duration_seconds = ? - WHERE id = ? - """, - ( - status.value, - json.dumps([asdict(s) for s in after_snapshot]), - error_message, - duration, - install_id, - ), - ) - - conn.commit() + start_time = datetime.datetime.fromisoformat(result[1]) + duration = (datetime.datetime.now() - start_time).total_seconds() + + # Create after snapshot + after_snapshot = self._create_snapshot(packages) + + # Update record + cursor.execute( + """ + UPDATE installations + SET status = ?, + after_snapshot = ?, + error_message = ?, + duration_seconds = ? + WHERE id = ? + """, + ( + status.value, + json.dumps([asdict(s) for s in after_snapshot]), + error_message, + duration, + install_id, + ), + ) + + conn.commit() logger.info(f"Installation {install_id} updated: {status.value}") except Exception as e: logger.error(f"Failed to update installation: {e}") raise
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 3268-3278: The tarball-helper subcommand currently performs
installs by default; update the tarball_parser to add a boolean flag "--execute"
(store_true) and make the "install-deps" action a dry-run unless args.execute is
True: modify parsing to include tarball_parser.add_argument("--execute",
action="store_true", help="Actually perform changes; default is dry-run") and
gate the logic in the handler that processes args.action == "install-deps" (and
the analogous handler block around the other occurrence referenced) to only run
the real install when args.execute is true, otherwise log/simulate the changes
and exit without making modifications.
- Around line 3931-3941: The file re-registers the same subparser twice, causing
argparse ValueError: conflicting subparser; remove the duplicate registration by
deleting or guarding the second call that creates tarball_parser via
subparsers.add_parser("tarball-helper", ...), and ensure only the first
tarball_parser definition (and its add_argument calls for "action" and "path")
remains; if conditional behavior is required wrap the second block with a check
to avoid re-adding the same subparser.
In `@cortex/tarball_helper.py`:
- Around line 83-89: The suggest_apt_packages function currently always prefixes
dependencies with "lib", producing invalid names like "liblibcurl-dev"; change
the logic in suggest_apt_packages so that if dep.lower().startswith("lib") you
only append "-dev" (pkg = f"{dep.lower()}-dev"), otherwise prefix with "lib" and
then append "-dev" (pkg = f"lib{dep.lower()}-dev"), keeping the rest of the
mapping logic unchanged; update references to the mapping variable if needed.
- Around line 30-113: Add missing return type hints and docstrings for the
public APIs and side-effecting methods: annotate _save_tracked_packages,
install_deps, track, and cleanup to return None; add concise one-line docstrings
to analyze, suggest_apt_packages, install_deps, track, and cleanup describing
their purpose and side effects; keep existing type hints for
_load_tracked_packages and _parse_dependencies as-is and ensure docstrings
mention parameters and return types for analyze and suggest_apt_packages so the
public interface is documented (refer to methods analyze, suggest_apt_packages,
install_deps, track, cleanup, and _save_tracked_packages).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 3931-3944: The function add_tarball_helper_subparser lacks type
annotations; update its signature to include proper type hints for the
subparsers parameter and return type, and annotate local variables where
appropriate (e.g., tarball_parser) to match project guidelines. Specifically,
import and use the appropriate typing (e.g., argparse.ArgumentParser or
argparse._SubParsersAction) and annotate the function as def
add_tarball_helper_subparser(subparsers: SubParsersType) -> None, ensuring
parameters passed to subparsers.add_parser and tarball_parser.add_argument are
correctly typed. Keep the function body unchanged aside from adding the
annotations on add_tarball_helper_subparser, subparsers, and tarball_parser to
satisfy the codebase typing conventions.
♻️ Duplicate comments (1)
cortex/cli.py (1)
3268-3277: Make tarball-helper installs dry-run by default.
install-depscurrently executes package installs immediately. CLI guidelines require dry-run by default and an explicit execution flag. Please add--executeand gate the install path. As per coding guidelines, implement dry-run by default for installation operations.🛠️ Proposed fix (add --execute + gate install)
tarball_parser = subparsers.add_parser( "tarball-helper", help="Tarball/manual build helper (analyze, install-deps, cleanup)" ) tarball_parser.add_argument( "action", choices=["analyze", "install-deps", "cleanup"], help="Action to perform" ) tarball_parser.add_argument( "path", nargs="?", help="Path to source directory (for analyze/install-deps)" ) + tarball_parser.add_argument( + "--execute", action="store_true", help="Execute installs (default: dry-run)" + )def add_tarball_helper_subparser(subparsers): # Avoid duplicate subparser registration if "tarball-helper" in subparsers.choices: return tarball_parser = subparsers.add_parser( "tarball-helper", help="Tarball/manual build helper (analyze, install-deps, cleanup)" ) tarball_parser.add_argument( "action", choices=["analyze", "install-deps", "cleanup"], help="Action to perform" ) tarball_parser.add_argument( "path", nargs="?", help="Path to source directory (for analyze/install-deps)" ) + tarball_parser.add_argument( + "--execute", action="store_true", help="Execute installs (default: dry-run)" + )elif args.action == "install-deps": deps = helper.analyze(args.path or ".") mapping = helper.suggest_apt_packages(deps) - helper.install_deps(list(mapping.values())) + if not getattr(args, "execute", False): + table = Table(title="Apt packages to install (dry-run)") + table.add_column("Dependency") + table.add_column("Apt Package") + for dep, pkg in mapping.items(): + table.add_row(dep, pkg) + Console().print(table) + return 0 + helper.install_deps(list(mapping.values()))Also applies to: 3931-3967
…CLI refactor, regex, error handling, dry-run, tests, docstrings, type hints
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/tarball_helper.py`:
- Around line 86-105: The setup.py AST parsing currently checks for string
literals using ast.Str (in the block handling fname == "setup.py"); update the
AST walker inside the ast.parse/ast.walk section to accept ast.Constant for
Python 3.8+ by replacing the ast.Str check with a check for ast.Constant and
verifying elt.value is an instance of str before adding to deps (keep existing
checks for ast.Assign, install_requires, node.value being ast.List or ast.Tuple
and other symbols like elt and deps unchanged); this ensures compatibility while
preserving the existing fallback regex behavior in the except block.
In `@tests/test_tarball_helper.py`:
- Around line 1-88: The test file defines test functions that reference
TarballHelper and json before those imports, causing NameError; move the module
docstring and the imports (json, os, shutil, tempfile, pytest, and from
cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper) to the very top
of the file before any test_* functions, and remove the duplicated docstring and
the repeated imports currently placed after the tests (the block starting around
the triple-quoted string and the imports at the end) so all tests can access
TarballHelper and json when they run.
♻️ Duplicate comments (9)
cortex/tarball_helper.py (6)
50-53: Add return type hint and docstring.
_save_tracked_packagesis missing a return type hint (-> None) and a docstring. As per coding guidelines, type hints and docstrings are required.♻️ Suggested fix
- def _save_tracked_packages(self): + def _save_tracked_packages(self) -> None: + """Persist tracked packages to disk.""" MANUAL_TRACK_FILE.parent.mkdir(parents=True, exist_ok=True)
55-74: Validate path exists before walking.If a non-existent or invalid path is provided,
os.walk()silently returns an empty iterator, resulting in no dependencies and no user feedback. Consider validating the path upfront.♻️ Suggested fix
def analyze(self, path: str) -> list[str]: """Analyze build files for dependencies.""" + if not os.path.isdir(path): + console.print(f"[red]Error:[/red] Path does not exist or is not a directory: {path}") + return [] deps = set() for root, _, files in os.walk(path):
111-121: Consider adding overrides for common package name mismatches.The simple heuristic works for generic cases but will produce incorrect package names for well-known libraries (e.g.,
OpenSSL→libopenssl-devinstead of the correctlibssl-dev). An override dictionary for common cases would improve user experience.
123-135: LGTM on tracking logic; consider sanitizing package names.The fix to only track successful installations is correct. However, package names derived from parsing should be validated against a safe character set (e.g.,
^[a-z0-9][a-z0-9+.-]*$) before passing to shell commands to prevent potential injection from malformed build files.
144-153: Add user confirmation before purging packages.The
cleanup()method runssudo apt-get purgewithout prompting. This could lead to accidental removal of packages. Consider adding a confirmation prompt or a--forceflag.
10-14: Docstring mentions "track" command that doesn't exist in CLI.The usage section documents
cortex tarball-helper track <package>, but the CLI parser (lines 159-168) only definesanalyze,install-deps, andcleanupsubcommands. Either add thetracksubcommand or remove it from the docstring.tests/test_tarball_helper.py (3)
82-86: Remove unused imports.
os,shutil,tempfile, andpytestare imported but not used in the tests. Remove them to keep the test file clean.♻️ Suggested fix
import json -import os -import shutil -import tempfile - -import pytest from cortex.tarball_helper import MANUAL_TRACK_FILE, TarballHelper
124-126: Mock doesn't verify subprocess arguments.The lambda mock ignores all arguments without verification. Consider using
unittest.mock.callor capturing arguments to assert thatapt-get purgeis called with the correct package names.♻️ Suggested improvement
# Simulate cleanup (mock subprocess) - monkeypatch.setattr("subprocess.run", lambda *a, **k: None) + calls = [] + monkeypatch.setattr("subprocess.run", lambda *a, **k: calls.append(a)) helper.cleanup() assert helper.tracked_packages == [] + assert any("libfoo-dev" in str(c) for c in calls)
91-130: Consider adding tests for install_deps success path.The test suite covers parsing, mapping, tracking, and cleanup workflows well. However, there's no test for the successful installation path in
install_deps()that verifies packages are tracked whenreturncode == 0.♻️ Example test for success path
def test_install_deps_success(tmp_path, monkeypatch): test_file = tmp_path / "manual_builds.json" monkeypatch.setattr("cortex.tarball_helper.MANUAL_TRACK_FILE", test_file) helper = TarballHelper() class SuccessResult: returncode = 0 monkeypatch.setattr("subprocess.run", lambda *a, **k: SuccessResult()) helper.install_deps(["libtest-dev"]) assert "libtest-dev" in helper.tracked_packages
…s for PR cortexlinux#641. All tests and style checks green.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 4001-4008: Wrap the interactive confirmation call in a try/except
around the input(...) in the cleanup flow so EOFError and KeyboardInterrupt are
caught and treated as a cancellation; if an exception occurs, call
Console().print("[yellow]Cleanup cancelled.[/yellow]") and do not call
helper.cleanup(), otherwise proceed to check confirm.lower() == "y" to run
helper.cleanup(); this change should be applied where pkgs_str is built and
input(...) is invoked (refer to helper.tracked_packages, input call,
helper.cleanup(), and Console().print).
In `@tests/test_tarball_helper.py`:
- Around line 95-110: Remove the duplicated test function definition for
test_install_deps_error_handling to resolve the CI F811 redefinition error; keep
a single copy (either the one at Line 68 or the one shown here) that asserts
TarballHelper.install_deps properly handles subprocess failures and updates
helper.tracked_packages, and delete the other duplicate function so only one
test_install_deps_error_handling exists referencing TarballHelper and its
install_deps behavior.
♻️ Duplicate comments (3)
cortex/cli.py (1)
3942-3962: Drop the redundantadd_tarball_helper_subparserdefinition.This redefines the same helper after
parse_args()and is effectively dead code because the subparser already exists. Remove the duplicate to avoid shadowing and confusion.Suggested fix
- def add_tarball_helper_subparser(subparsers: argparse._SubParsersAction) -> None: - """Add tarball-helper subparser if not already present.""" - if "tarball-helper" in subparsers.choices: - return - tarball_parser = subparsers.add_parser( - "tarball-helper", help="Tarball/manual build helper (analyze, install-deps, cleanup)" - ) - tarball_parser.add_argument( - "action", choices=["analyze", "install-deps", "cleanup"], help="Action to perform" - ) - tarball_parser.add_argument( - "path", nargs="?", help="Path to source directory (for analyze/install-deps)" - ) - tarball_parser.add_argument( - "--execute", - action="store_true", - help="Actually install dependencies (default: dry-run)", - ) - - add_tarball_helper_subparser(subparsers)cortex/tarball_helper.py (2)
10-14: Usage docstring advertises unsupportedtrackaction.Line 13 lists
cortex tarball-helper track <package>, but the CLI only supports analyze/install-deps/cleanup. Update the usage text or implement the command to avoid confusion.Suggested fix
cortex tarball-helper analyze <path> cortex tarball-helper install-deps <path> - cortex tarball-helper track <package> cortex tarball-helper cleanup
30-31: Add missing return type hints.
__init__and_save_tracked_packagesshould declare-> Noneto satisfy the project’s type-hint requirement. As per coding guidelines, type hints are required.Suggested fix
-class TarballHelper: - def __init__(self): +class TarballHelper: + def __init__(self) -> None: self.tracked_packages = self._load_tracked_packages() @@ - def _save_tracked_packages(self): + def _save_tracked_packages(self) -> None: MANUAL_TRACK_FILE.parent.mkdir(parents=True, exist_ok=True) with open(MANUAL_TRACK_FILE, "w") as f: json.dump({"packages": self.tracked_packages}, f, indent=2)Also applies to: 83-86
| pkgs_str = ", ".join(helper.tracked_packages) | ||
| confirm = input( | ||
| f"Are you sure you want to purge the following packages? {pkgs_str} [y/N]: " | ||
| ) | ||
| if confirm.lower() == "y": | ||
| helper.cleanup() | ||
| else: | ||
| Console().print("[yellow]Cleanup cancelled.[/yellow]") |
There was a problem hiding this comment.
Handle non-interactive cleanup confirmations safely.
input() can raise EOFError/KeyboardInterrupt in non-interactive contexts, which would crash the CLI. Guard it and treat it as a cancellation.
Suggested fix
- confirm = input(
- f"Are you sure you want to purge the following packages? {pkgs_str} [y/N]: "
- )
- if confirm.lower() == "y":
- helper.cleanup()
- else:
- Console().print("[yellow]Cleanup cancelled.[/yellow]")
+ try:
+ confirm = input(
+ f"Are you sure you want to purge the following packages? {pkgs_str} [y/N]: "
+ )
+ except (EOFError, KeyboardInterrupt):
+ Console().print("[yellow]Cleanup cancelled.[/yellow]")
+ return 0
+ if confirm.lower() == "y":
+ helper.cleanup()
+ else:
+ Console().print("[yellow]Cleanup cancelled.[/yellow]")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pkgs_str = ", ".join(helper.tracked_packages) | |
| confirm = input( | |
| f"Are you sure you want to purge the following packages? {pkgs_str} [y/N]: " | |
| ) | |
| if confirm.lower() == "y": | |
| helper.cleanup() | |
| else: | |
| Console().print("[yellow]Cleanup cancelled.[/yellow]") | |
| pkgs_str = ", ".join(helper.tracked_packages) | |
| try: | |
| confirm = input( | |
| f"Are you sure you want to purge the following packages? {pkgs_str} [y/N]: " | |
| ) | |
| except (EOFError, KeyboardInterrupt): | |
| Console().print("[yellow]Cleanup cancelled.[/yellow]") | |
| return 0 | |
| if confirm.lower() == "y": | |
| helper.cleanup() | |
| else: | |
| Console().print("[yellow]Cleanup cancelled.[/yellow]") |
🤖 Prompt for AI Agents
In `@cortex/cli.py` around lines 4001 - 4008, Wrap the interactive confirmation
call in a try/except around the input(...) in the cleanup flow so EOFError and
KeyboardInterrupt are caught and treated as a cancellation; if an exception
occurs, call Console().print("[yellow]Cleanup cancelled.[/yellow]") and do not
call helper.cleanup(), otherwise proceed to check confirm.lower() == "y" to run
helper.cleanup(); this change should be applied where pkgs_str is built and
input(...) is invoked (refer to helper.tracked_packages, input call,
helper.cleanup(), and Console().print).
| def test_install_deps_error_handling(monkeypatch): | ||
| helper = TarballHelper() | ||
| called = [] | ||
|
|
||
| def fake_run(args, check): | ||
| called.append(args) | ||
|
|
||
| class Result: | ||
| returncode = 1 | ||
|
|
||
| return Result() | ||
|
|
||
| monkeypatch.setattr("subprocess.run", fake_run) | ||
| helper.tracked_packages = [] | ||
| helper.install_deps(["libfail-dev"]) | ||
| assert "libfail-dev" not in helper.tracked_packages |
There was a problem hiding this comment.
Remove duplicate test definition (CI F811).
CI reports F811 because test_install_deps_error_handling is redefined at Line 95, overriding the earlier definition at Line 68. Remove one copy to restore lint/test health.
Suggested fix
-def test_install_deps_error_handling(monkeypatch):
- helper = TarballHelper()
- called = []
-
- def fake_run(args, check):
- called.append(args)
-
- class Result:
- returncode = 1
-
- return Result()
-
- monkeypatch.setattr("subprocess.run", fake_run)
- helper.tracked_packages = []
- helper.install_deps(["libfail-dev"])
- assert "libfail-dev" not in helper.tracked_packages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_install_deps_error_handling(monkeypatch): | |
| helper = TarballHelper() | |
| called = [] | |
| def fake_run(args, check): | |
| called.append(args) | |
| class Result: | |
| returncode = 1 | |
| return Result() | |
| monkeypatch.setattr("subprocess.run", fake_run) | |
| helper.tracked_packages = [] | |
| helper.install_deps(["libfail-dev"]) | |
| assert "libfail-dev" not in helper.tracked_packages |
🧰 Tools
🪛 GitHub Actions: CI
[error] 95-95: F811 Redefinition of unused test_install_deps_error_handling from line 68 in tests/test_tarball_helper.py.
🪛 GitHub Check: Lint
[failure] 95-95: Ruff (F811)
tests/test_tarball_helper.py:95:5: F811 Redefinition of unused test_install_deps_error_handling from line 68: test_install_deps_error_handling redefined here
🤖 Prompt for AI Agents
In `@tests/test_tarball_helper.py` around lines 95 - 110, Remove the duplicated
test function definition for test_install_deps_error_handling to resolve the CI
F811 redefinition error; keep a single copy (either the one at Line 68 or the
one shown here) that asserts TarballHelper.install_deps properly handles
subprocess failures and updates helper.tracked_packages, and delete the other
duplicate function so only one test_install_deps_error_handling exists
referencing TarballHelper and its install_deps behavior.
|
@aybanda Will handle this internally, thanks for working. |
2 participants
Related Issue
Closes #452
Summary
Adds the tarball/manual build helper CLI and tests, making it easier to analyze, install, and clean up dependencies for manual or tarball-based builds.
AI Disclosure
AI/IDE/Agents used (Used GitHub Copilot and AI agent for code, linting, and formatting assistance)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.